Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add optional chaining in the event CSS is null #472

Closed
wants to merge 2 commits into from

Conversation

michael-siek
Copy link

@michael-siek michael-siek commented Mar 5, 2024

Adding this in the event that CSS global is null this doesn't throw an error. There was a user that went thru this code path and CSS was null and threw an error.

Copy link

netlify bot commented Mar 5, 2024

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit 874a4d6
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/65e76546294308000817faa4
😎 Deploy Preview https://deploy-preview-472--colorjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@LeaVerou
Copy link
Member

LeaVerou commented Mar 6, 2024

Hey, thank you so much for working on this!
Just curious, in what situation would CSS be null?

@LeaVerou
Copy link
Member

LeaVerou commented Mar 6, 2024

I wonder if a better solution might be to do globalThis.CSS && CSS.supports for the check

@michael-siek
Copy link
Author

Hey @LeaVerou I'm unsure on the exact reason why but we are getting a TypeError: Cannot read properties of null (reading 'supports'). I went with optional chaining to keep consistent with the current code.

@michael-siek
Copy link
Author

Hey @LeaVerou is there any changes that you would like for me to do to this PR to solve the issue?

@michael-siek
Copy link
Author

Hey @LeaVerou any update on this issue?

@LeaVerou
Copy link
Member

LeaVerou commented Mar 27, 2024

Hi there! Sorry for the delay. I’m just not sure in what case CSS would be present, but null.

If it's something that happens naturally in some case or in some environment, sure, I'd be happy to merge this, but when I asked the response was that you didn’t know why CSS was null, it just was. If some part of your code (or some library you're using) is overwriting CSS, Color.js is not the place to fix this. It's a slippery slope, we can't start adding optional chaining to every single global just in case it was overwritten.

@straker
Copy link

straker commented Apr 1, 2024

@LeaVerou We did some digging and found that users are following this guide for jest-preset-angular in which they are being told to override CSS with null.

@svgeesus
Copy link
Member

svgeesus commented Apr 3, 2024

@LeaVerou We did some digging and found that users are following this guide for jest-preset-angular in which they are being told to override CSS with null.

That very much sounds like it should be fixed at the point which is causing this problem, which I see has been raised as an issue.

@svgeesus svgeesus closed this Apr 3, 2024
@LeaVerou
Copy link
Member

LeaVerou commented Apr 4, 2024

I think I'm tending to agree. @straker as a workaround you can check if it's null and set it to undefined if so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants